-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom TransactionSigner and eth-account module improvements. #2367
Conversation
…ore-method module updated. Anything is still in a POC phase
This reverts commit 4b174ed.
…ted in Accounts module and TransactionSigner simplified
…nd dependency handling updated in the core-method module.
…d just works if you're connected to a Parity node
this.methodFactory = this.contractModuleFactory.createMethodFactory(); | ||
this.abiModel = this.abiMapper.map(abi); | ||
this.transactionSigner = options.transactionSigner; | ||
this.options = options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicates with line 62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a duplication. I'm just passing the TransactionSigner
dependency over the options. Check out the Eth module for seeing the real reason why I'm doing this. The complete dependency handling will get improved later (removing of the Factory objects etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this assignment this.options = options;
it happens twice probably a left over from git merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I got it wrong. Thanks.
@nivida Any great example on how to use it? |
this.confirmationSubscription = this.subscriptionsFactory | ||
.createNewHeadsSubscription(moduleInstance) | ||
.subscribe(() => { | ||
console.log('newHEAD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe forgot to remove?
Description
This PR fixes #2356 #2378 #1998 #1255 #2300 and will also improve the QA of the module.
Web3 Accounts Refactoring
The eth-accounts module is one of the last modules which didn't got improved during the last refactoring of the core modules.
My idea now is to clean up the eth-accounts module and to add a
transactionSigner
options property for the eth module.This sounds like a big refactoring but I will mostly just move the things.
TransactionSigner
It will be possible to create your own class of type
TransactionSigner
and inject it over the options property. The class nameTransactionSigner
is a reserved class name and will be used for the internal signer class.Example
Accounts
The interface of the Accounts class (
web3.eth.accounts
) will still be the same.Type of change
Checklist:
npm run test
in the root folder with success and extended the tests if necessary.npm run build
in the root folder and tested it in the browser and with node.npm run dtslint
in the root folder and tested that all my types are correct